Skip to content

Conversation

mahlau-flex
Copy link
Contributor

@mahlau-flex mahlau-flex commented Oct 9, 2025

Fixed all (except one) of the many security vulnerabilities found by zizmor (12 informational, 0 low, 19 medium, 19 high). While I am pretty sure that the functionality remains the same, we will need to see (an carefully review) if something broke during these changes.

The one issue I could not change is the following (status informational):

info[use-trusted-publishing]: prefer trusted publishing for authentication
   --> .github/workflows/tidy3d-python-client-release.yml:121:9
    |
119 |       run: |
    |       --- this step
120 |         python -m build
121 |         python -m twine upload --repository pypi dist/*
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this command
    |
    = note: audit confidence → High

I have suppressed the zizmor warning for now. In the long run it would probably be best to use the newer authentication of pypi releases (according to Claude, one can just register a github workflow in pypi instead of using the legacy token system).

Additionally, I have added a pre-commit check for zizmor as well. This might seem a bit excessive, but runs extremely quickly so I think it will not bother anyone.

Lastly, I changed the configuration of zizmor itself in the github actions. I noticed that in the last PR, the job completed successfully, even though it found a security issue. This is, because it uploads the results to github advanced security, which then makes a comment showing the issue. But, this does not prevent merging and is not clearly visibly. With the new system, the checks themselves should (hopefully) fail.

Greptile Overview

Updated On: 2025-10-09 19:03:13 UTC

Summary

This PR implements comprehensive security hardening for GitHub Actions workflows based on findings from the zizmor security scanner, which identified 50 security vulnerabilities (12 informational, 19 medium, 19 high) across the CI/CD pipeline. The changes follow GitHub Actions security best practices without altering functionality.

Key security improvements include:

  1. Permission Management: Moving from broad permissions to the principle of least privilege by setting global permissions to contents: read and granting specific permissions only where needed (e.g., contents: write for jobs that push changes)

  2. Action Security: Pinning action versions to specific commit SHAs instead of floating tags to prevent supply chain attacks where malicious code could be injected through compromised action updates

  3. Credential Protection: Adding persist-credentials: false to checkout actions to prevent GitHub tokens from being accessible to subsequent workflow steps that don't need them

  4. Script Injection Prevention: Moving GitHub context expressions (like PR titles and branch names) to environment variables to prevent potential script injection attacks

  5. Proactive Security: Adding zizmor as both a dependency in pyproject.toml and a pre-commit hook to catch future security issues during development

The changes span 8 workflow files covering testing, releases, documentation sync, and daily operations. One informational issue regarding PyPI trusted publishing was acknowledged but left unaddressed, with plans to migrate to the newer authentication system in the future. The implementation maintains all existing functionality while significantly reducing the attack surface of the CI/CD pipeline.

Important Files Changed

Changed Files
Filename Score Overview
.github/workflows/tidy3d-python-client-tests.yml 4/5 Comprehensive security hardening with action pinning, credential protection, and zizmor integration
.github/workflows/tidy3d-python-client-release.yml 4/5 Security improvements for release workflow with permission scoping and action pinning
.github/workflows/tidy3d-python-client-update-lockfile.yml 4/5 Security hardening for lockfile update workflow with SHA pinning and permission controls
.github/workflows/tidy3d-python-client-daily.yml 4/5 Permission restrictions and explicit secret passing for daily workflow security
.github/workflows/tidy3d-docs-sync-readthedocs-repo.yml 5/5 Clean security improvements with minimal permission grants for documentation sync
.github/workflows/tidy3d-python-client-submodules-test.yml 5/5 Simple security hardening with read-only permissions and credential protection
.github/workflows/tidy3d-python-client-develop-cli.yml 5/5 Standard security improvements with action pinning and permission restrictions
.pre-commit-config.yaml 4/5 Addition of zizmor pre-commit hook for proactive security scanning
pyproject.toml 5/5 Clean addition of zizmor dependency for security tooling integration

Confidence score: 4/5

  • This PR is safe to merge with careful monitoring of CI/CD pipeline functionality
  • Score reflects the comprehensive nature of security changes across critical workflow files that require validation
  • Pay close attention to all workflow files to ensure CI/CD operations continue functioning correctly

Sequence Diagram

sequenceDiagram
    participant User
    participant Dev as "Developer"
    participant GitHub as "GitHub Actions"
    participant PyPI
    participant ReadTheDocs as "ReadTheDocs Mirror"
    participant Zizmor as "Zizmor Security Tool"
    
    User->>Dev: "Request security fixes for GitHub Actions"
    Dev->>Zizmor: "Run security audit on .github/workflows/*"
    Zizmor-->>Dev: "Report 12 informational, 19 medium, 19 high vulnerabilities"
    
    Dev->>GitHub: "Update workflow files with security fixes"
    Note over Dev,GitHub: Pin action versions to specific commit hashes<br/>Add persist-credentials: false<br/>Set minimal permissions<br/>Update to latest action versions
    
    Dev->>GitHub: "Add zizmor pre-commit hook"
    GitHub->>Zizmor: "Run zizmor check on workflow files"
    Zizmor-->>GitHub: "Security validation results"
    
    Dev->>GitHub: "Configure zizmor to fail CI on security issues"
    Note over GitHub: Previously uploaded to Advanced Security<br/>Now fails the check directly
    
    GitHub->>PyPI: "Release workflow with suppressed trusted publishing warning"
    Note over GitHub,PyPI: Legacy token authentication maintained<br/>zizmor warning suppressed
    
    GitHub->>ReadTheDocs: "Sync documentation with security fixes"
    
    Dev->>User: "Security fixes complete (except trusted publishing)"
    Note over User,Dev: All issues fixed except one informational<br/>about using trusted publishing for PyPI
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

@mahlau-flex mahlau-flex force-pushed the hotfix/fix-security-issues branch 3 times, most recently from 94bc1d5 to 303c5eb Compare October 9, 2025 19:21
Copy link
Contributor

github-actions bot commented Oct 9, 2025

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

@mahlau-flex mahlau-flex changed the title ci: security fixes in github actions as suggested by zizmor FXC-347: security fixes in github actions as suggested by zizmor Oct 10, 2025
@mahlau-flex
Copy link
Contributor Author

I have renamed the title of this PR to link to the Zizmor jira task. Are we supposed to link multiple PRs to a single jira issue or should we create a new one everytime?

@mahlau-flex mahlau-flex changed the title FXC-347: security fixes in github actions as suggested by zizmor FXC-3603: security fixes in github actions as suggested by zizmor Oct 10, 2025
Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mahlau-flex this is great! Besides what was already commented LGTM. I'm not too familiar with the details but as long as everything keeps running it looks fine to me.

@yaugenst-flex yaugenst-flex force-pushed the hotfix/fix-security-issues branch from 303c5eb to 726ec9f Compare October 14, 2025 09:24
@mahlau-flex
Copy link
Contributor Author

hm, it seems that some tests are actually failing here. It looks like these are the discussed windows test cases. I will investigate how to fix this

@daquinteroflex
Copy link
Collaborator

Hi @mahlau-flex thanks! Could you share some github actions run links where you test each workflow bar the release one?

Copy link
Collaborator

@daquinteroflex daquinteroflex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @mahlau-flex this is a good cleanup. Let's just do some final checking on the workflow runs and then we can merge

@mahlau-flex
Copy link
Contributor Author

Hi @mahlau-flex thanks! Could you share some github actions run links where you test each workflow bar the release one?

Sorry, I am not sure what you mean by that. Is it possible to manually trigger the workflows as a test? Would this break anything?

@mahlau-flex mahlau-flex force-pushed the hotfix/fix-security-issues branch from 726ec9f to 609d153 Compare October 15, 2025 06:29
@daquinteroflex
Copy link
Collaborator

So yeah if you manually run all bar the release action from this branch it should be fine, it's just to check all works nicely now

@mahlau-flex mahlau-flex force-pushed the hotfix/fix-security-issues branch 5 times, most recently from 0fe4e27 to f0971b9 Compare October 16, 2025 13:34
@mahlau-flex mahlau-flex force-pushed the hotfix/fix-security-issues branch 2 times, most recently from 7ff8f9d to 92cd2cd Compare October 16, 2025 13:41
@mahlau-flex
Copy link
Contributor Author

Everything should be working now. Here are links to the successfull manual runs:

The daily run fails, but I believe this has nothing to do with the changes here, but rather that the test is doing what its supposed to be doing and alerting that we need to update a submodule:
https://github.com/flexcompute/tidy3d/actions/runs/18563335630

Copy link
Collaborator

@daquinteroflex daquinteroflex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

@mahlau-flex mahlau-flex force-pushed the hotfix/fix-security-issues branch from 92cd2cd to 2555b7d Compare October 16, 2025 13:57
@yaugenst-flex yaugenst-flex added this pull request to the merge queue Oct 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Oct 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants